feat(instances): time-space and raid entrances, from the capture to w… - #2282

No actionable comments were generated in the recent review. 🎉

Walkthrough

The change adds scripted-instance metadata, XML definition parsing, packet import, isolated room-based runs, entry handling, map lifecycle cleanup, persistence updates, and localized messages.

Changes

Scripted instance lifecycle

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🟡 Moderate · up to cc39b

This PR adds private instance entry and level-gated entrances, but party members may not share the same rooms, displayed requirements can conflict, and raid entrances may be unusable because their level range is stored as 0–0. These issues should be fixed or explicitly accepted before merging.

Sequence Diagram(s)

sequenceDiagram
  participant Character
  participant TreqPacketHandler
  participant IScriptedInstanceService
  participant MapInstanceGenerator
  participant MapChangeService
  Character->>TreqPacketHandler: Request time-space entry
  TreqPacketHandler->>IScriptedInstanceService: Resolve entrance and instantiate run
  IScriptedInstanceService->>MapInstanceGenerator: Create and start configured rooms
  MapInstanceGenerator-->>IScriptedInstanceService: Return room map instance IDs
  IScriptedInstanceService-->>TreqPacketHandler: Return ScriptedInstanceRun
  TreqPacketHandler->>MapChangeService: Move character to first room
  MapChangeService->>IScriptedInstanceService: Dispose run if empty
  MapChangeService-->>Character: Complete map change

• Fix all pre-merge checks with AI

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Pushed a follow-up. Comments cut (371 lines), and the three new resource strings are now actually translated in all eight non-English files rather than left in English.

CodeRabbit found three real ones and I fixed them:

• The service was transient. The convention scan registers discovered services as transient unless they implement ISingletonService, and it runs after the explicit AddSingleton — so every resolution started with an empty run index and no run was ever found or disposed. Marked, and the explicit registration removed.
• A run could be destroyed mid-transfer. Walking between two rooms of the same instance leaves the character in neither for a moment, and the empty check ran in that window — tearing down the room they were walking into. It now runs after the destination session is registered.
• treq trusted the coordinates. A modified client could name any entrance on the map. It now requires the character to be standing next to the one it names.

Also: a zero level ceiling is treated as no ceiling, so rows that predate the column do not turn everyone away.

On the XML — you are right, and I would rather not defend it. I used the OpenNos shape only because it is what existing hand-written instance content is written in, but that is an argument for a one-off importer, not for the runtime format.

What would you like instead? My inclination, if you have no strong preference, is definitions as C# in the codebase — a small fluent builder per instance, compiled, greppable, refactorable, and testable, with no parser and no string-typed room references. It costs the ability to hot-edit an instance without a rebuild; if that matters to you, JSON files under configuration/ with a schema would keep it.

Happy to split this PR if you prefer: the entrance import, the minimap markers and the entry panel do not depend on the format at all, and the run/room machinery could follow once the format is settled.

This is also far too big PR

Following up on the XML — I built the alternative rather than leaving the question hanging, so there is something to look at. A definition now reads like this:

InstanceDefinitionBuilder
    .Named(id: 3, label: "Cuby", title: "Mother Cuby")
    .ForLevels(20, 45)
    .WithLives(3)
    .StartingAt(12, 34)
    .Rewarding(gold: 15000, reputation: 200)
    .WithRoom(2004, out var entrance)
    .WithRoom(2005, out var lair, indexX: 1)
    .Requiring(1000, 2)
    .Drawing(1012, 3, design: 7, randomRare: true)
    .WithSpecialReward(2282, 1, heroic: true)
    .Build();

Room keys are handed out rather than written down, which is the part the XML could not do: there, the same number had to be typed in the room and again in everything pointing at it, and nothing checked the two agreed. Here a stale reference is a build error.

Being straight about the state: the builder is written and tested, but nothing uses it yet — ScriptedInstanceService still calls ScriptedInstanceDefinitionParser.Parse(row.Script). So right now the PR carries a proposal next to the thing you objected to, and the thing you objected to is still the live path. That is not a state worth merging.

What I would do next, unless you say otherwise. You asked on #2281 for smaller PRs, and this splits cleanly along that line:

1. Entrance import, minimap markers, entry panel. Does not depend on the definition format at all — wp/rbr from the capture, treq, the level-range checks. This is most of the diff and none of the argument.
2. Run and room machinery, on whichever format you settle on.

That gets the uncontroversial part reviewable on its own and leaves the format question in a small PR you can take your time over.

Two things I would still like your call on, because guessing them wastes both our time:

• C# or JSON? C# gets compiler checking and refactoring; JSON under configuration/ keeps the ability to change an instance without a rebuild. I lean C# but I do not have to live with the content pipeline — you do.
• The existing XML content. There is hand-written instance content in that format. I would keep the parser as a one-off importer that emits definitions, clearly not the runtime path — but if you would rather that content be rewritten, the parser goes entirely.

Every one of #2292's 19 files is also in this PR's 34, so the two overlap completely rather than stacking. Both are green, but merging one will conflict the other.

Which is the base? If #2292 is the import half split out of this one, I'd merge it first and rebase this on top so the migration lands once. If this PR supersedes it, #2292 should close.

Not reviewing the content further until that's settled — no point reading the same migration twice.

#2292 is the base. This PR was written whole and #2292 is the import half split out of it afterwards, so they both branched from master rather than stacking — which is exactly the complete overlap you measured.

I've rebuilt this branch on top of #2292 rather than leaving the question open. Its three commits are now #2292's two plus one, so the moment #2292 merges this collapses to the fifteen runtime files: the service, the run, the definition builder, TreqPacketHandler, the two map hooks and their tests.

One thing worth knowing, because it decides which copy survives: the two had diverged. A plain rebase --onto conflicted in four files — each branch had taken review fixes the other hadn't. Rebuilding on #2292 keeps the version reviewed there, including the backfill fix where a stored entrance was skipped instead of having its metadata refreshed. That fix only exists on #2292, so merging in the other order would have lost it.

Two language keys came across because only the runtime uses them, TIMESPACE_LEVEL_NOT_ALLOWED and TIMESPACE_NOT_AVAILABLE, translated in all nine resource files.

Solution builds with zero warnings; NosCore.GameObject.Tests 409/409, NosCore.Parser.Tests 110/110. No play test — I don't run the servers.